From fbafb2eb228219381d8134feb80441ed766261d4 Mon Sep 17 00:00:00 2001
From: jo <ljonas@riseup.net>
Date: Thu, 18 Jul 2024 10:40:42 +0200
Subject: [PATCH] chore: do not retry on HTTP 429 or HTTP 503

Related to #470

We prefer not to retry requests:
- If the ingress return a "bare" 429, as the backend was not able to respond anyway.
- If the ingress return a "bare" 503, to prevent possible self made ddos, and preventing a disaster recovery.
---
 hcloud/action_waiter_test.go        |  2 +-
 hcloud/client_handler_retry.go      |  5 +----
 hcloud/client_handler_retry_test.go | 25 ++++++++++++++-----------
 hcloud/hcloud.go                    |  2 --
 4 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/hcloud/action_waiter_test.go b/hcloud/action_waiter_test.go
index 7ad84988..1202c056 100644
--- a/hcloud/action_waiter_test.go
+++ b/hcloud/action_waiter_test.go
@@ -99,7 +99,7 @@ func TestWaitFor(t *testing.T) {
 				Name: "succeed with retry",
 				WantRequests: []mockutils.Request{
 					{Method: "GET", Path: "/actions?id=1509772237&page=1&sort=status&sort=id",
-						Status: 503,
+						Status: 502,
 					},
 					{Method: "GET", Path: "/actions?id=1509772237&page=1&sort=status&sort=id",
 						Status: 200,
diff --git a/hcloud/client_handler_retry.go b/hcloud/client_handler_retry.go
index 342012ca..b0983c96 100644
--- a/hcloud/client_handler_retry.go
+++ b/hcloud/client_handler_retry.go
@@ -69,11 +69,8 @@ func retryPolicy(resp *Response, err error) bool {
 			}
 		case errors.Is(err, ErrStatusCode):
 			switch resp.Response.StatusCode {
-			// 4xx errors
-			case http.StatusTooManyRequests:
-				return true
 			// 5xx errors
-			case http.StatusBadGateway, http.StatusServiceUnavailable, http.StatusGatewayTimeout:
+			case http.StatusBadGateway, http.StatusGatewayTimeout:
 				return true
 			}
 		case errors.As(err, &netErr):
diff --git a/hcloud/client_handler_retry_test.go b/hcloud/client_handler_retry_test.go
index da78f583..265f7d25 100644
--- a/hcloud/client_handler_retry_test.go
+++ b/hcloud/client_handler_retry_test.go
@@ -31,11 +31,11 @@ func TestRetryHandler(t *testing.T) {
 			},
 		},
 		{
-			name: "http 503 error recovery",
+			name: "http 502 error recovery",
 			wrapped: func(req *http.Request, _ any) (*Response, error) {
-				resp := fakeResponse(t, 503, "", false)
+				resp := fakeResponse(t, 502, "", false)
 				resp.Response.Request = req
-				return resp, fmt.Errorf("%w %d", ErrStatusCode, 503)
+				return resp, fmt.Errorf("%w %d", ErrStatusCode, 502)
 			},
 			recover: true,
 			want: func(t *testing.T, err error, retryCount int) {
@@ -44,14 +44,14 @@ func TestRetryHandler(t *testing.T) {
 			},
 		},
 		{
-			name: "http 503 error",
+			name: "http 502 error",
 			wrapped: func(req *http.Request, _ any) (*Response, error) {
-				resp := fakeResponse(t, 503, "", false)
+				resp := fakeResponse(t, 502, "", false)
 				resp.Response.Request = req
-				return resp, fmt.Errorf("%w %d", ErrStatusCode, 503)
+				return resp, fmt.Errorf("%w %d", ErrStatusCode, 502)
 			},
 			want: func(t *testing.T, err error, retryCount int) {
-				assert.EqualError(t, err, "server responded with status code 503")
+				assert.EqualError(t, err, "server responded with status code 502")
 				assert.Equal(t, 5, retryCount)
 			},
 		},
@@ -116,8 +116,11 @@ func TestRetryPolicy(t *testing.T) {
 		want bool
 	}{
 		{
-			// The API error code unavailable is used in many unexpected situations, we must only
-			// retry if the server returns HTTP 503.
+			name: "server returns 502 error",
+			resp: fakeResponse(t, 502, ``, false),
+			want: true,
+		},
+		{
 			name: "api returns unavailable error",
 			resp: fakeResponse(t, 503, `{"error":{"code":"unavailable"}}`, true),
 			want: false,
@@ -125,7 +128,7 @@ func TestRetryPolicy(t *testing.T) {
 		{
 			name: "server returns 503 error",
 			resp: fakeResponse(t, 503, ``, false),
-			want: true,
+			want: false,
 		},
 		{
 			name: "api returns rate_limit_exceeded error",
@@ -135,7 +138,7 @@ func TestRetryPolicy(t *testing.T) {
 		{
 			name: "server returns 429 error",
 			resp: fakeResponse(t, 429, ``, false),
-			want: true,
+			want: false,
 		},
 		{
 			name: "api returns conflict error",
diff --git a/hcloud/hcloud.go b/hcloud/hcloud.go
index ccdfe59d..bb911087 100644
--- a/hcloud/hcloud.go
+++ b/hcloud/hcloud.go
@@ -14,9 +14,7 @@ The following rules defines when a request can be retried:
 When the [http.Client] returned a network timeout error.
 
 When the API returned an HTTP error, with the status code:
-  - [http.StatusTooManyRequests]
   - [http.StatusBadGateway]
-  - [http.StatusServiceUnavailable]
   - [http.StatusGatewayTimeout]
 
 When the API returned an application error, with the code: