From 535c9cee2bb4dd0abc0d14d573271d7f68b5d02f Mon Sep 17 00:00:00 2001
From: Jonathan Gramain <jonathan.gramain@scality.com>
Date: Thu, 19 Dec 2024 14:18:58 -0800
Subject: [PATCH] impr: BKTCLT-35 go client: custom HTTP client

Add the possibility to attach a custom HTTP client to a new instance
of BucketClient.

The first intended use is to add a timeout to HTTP requests sent by
BucketClient.
---
 go/bucketclient.go             | 23 +++++++++++++--
 go/bucketclientrequest.go      |  2 +-
 go/bucketclientrequest_test.go | 52 +++++++++++++++++++++++++++++++++-
 3 files changed, 72 insertions(+), 5 deletions(-)

diff --git a/go/bucketclient.go b/go/bucketclient.go
index 75f1691..a168c83 100644
--- a/go/bucketclient.go
+++ b/go/bucketclient.go
@@ -1,12 +1,29 @@
 package bucketclient
 
+import (
+	"net/http"
+)
+
 type BucketClient struct {
-	Endpoint string
-	Metrics  *BucketClientMetrics
+	Endpoint   string
+	HTTPClient *http.Client
+	Metrics    *BucketClientMetrics
 }
 
+// New creates a new BucketClient instance, with the provided endpoint (e.g. "localhost:9000")
+// and the default HTTP client.
 func New(bucketdEndpoint string) *BucketClient {
 	return &BucketClient{
-		Endpoint: bucketdEndpoint,
+		Endpoint:   bucketdEndpoint,
+		HTTPClient: http.DefaultClient,
+	}
+}
+
+// NewWithHTTPClient creates a new BucketClient instance, with the provided endpoint
+// (e.g. "localhost:9000") using the provided http.Client instance.
+func NewWithHTTPClient(bucketdEndpoint string, httpClient *http.Client) *BucketClient {
+	return &BucketClient{
+		Endpoint:   bucketdEndpoint,
+		HTTPClient: httpClient,
 	}
 }
diff --git a/go/bucketclientrequest.go b/go/bucketclientrequest.go
index 8d38755..8bc9e83 100644
--- a/go/bucketclientrequest.go
+++ b/go/bucketclientrequest.go
@@ -115,7 +115,7 @@ func (client *BucketClient) Request(ctx context.Context,
 			if options.idempotent {
 				request.Header["Idempotency-Key"] = []string{}
 			}
-			response, err = http.DefaultClient.Do(request)
+			response, err = client.HTTPClient.Do(request)
 		}
 	}
 	if err != nil {
diff --git a/go/bucketclientrequest_test.go b/go/bucketclientrequest_test.go
index 29c4750..c9fe6f0 100644
--- a/go/bucketclientrequest_test.go
+++ b/go/bucketclientrequest_test.go
@@ -34,7 +34,7 @@ var _ = Describe("BucketClient.Request()", func() {
 		Expect(err).To(MatchError(ContainSubstring("no responder found")))
 	})
 
-	Context("with valid URL", Ordered, func() {
+	Context("with valid URL and default HTTP client", Ordered, func() {
 		It("succeeds with a 200 response on GET request", func(ctx SpecContext) {
 			httpmock.RegisterResponder(
 				"GET", "http://localhost:9000/default/bucket/somebucket/someobject",
@@ -148,4 +148,54 @@ var _ = Describe("BucketClient.Request()", func() {
 			Expect(err).To(MatchError(context.DeadlineExceeded))
 		})
 	})
+
+	Context("with valid URL and custom HTTP client with timeout", Ordered, func() {
+		var clientWithTimeout *bucketclient.BucketClient
+
+		BeforeAll(func() {
+			clientWithTimeout = bucketclient.NewWithHTTPClient("http://localhost:9000",
+				&http.Client{
+					Timeout: 1 * time.Second,
+				})
+		})
+
+		It("succeeds with a 200 response on GET request", func(ctx SpecContext) {
+			httpmock.RegisterResponder(
+				"GET", "http://localhost:9000/default/bucket/somebucket/someobject",
+				httpmock.NewStringResponder(200, `{"some":"metadata","version":"1234"}`),
+			)
+			Expect(clientWithTimeout.Request(ctx, "GetObject", "GET",
+				"/default/bucket/somebucket/someobject")).To(Equal(
+				[]byte(`{"some":"metadata","version":"1234"}`)))
+		})
+
+		It("times out after the configured delay without a response", func(ctx SpecContext) {
+			httpmock.RegisterResponder(
+				"GET", "http://localhost:9000/default/bucket/somebucket/someobject",
+				func(req *http.Request) (*http.Response, error) {
+					// respond after 2 seconds > timeout of one second
+					time.Sleep(2 * time.Second)
+					return httpmock.NewStringResponse(200,
+						`{"some":"metadata","version":"1234"}`), nil
+				},
+			)
+			startTime := time.Now()
+			_, err := clientWithTimeout.Request(ctx, "GetObject", "GET",
+				"/default/bucket/somebucket/someobject")
+			duration := time.Since(startTime)
+
+			Expect(err).ToNot(BeNil())
+			bcErr, isBCErr := err.(*bucketclient.BucketClientError)
+			Expect(isBCErr).To(BeTrue())
+			Expect(bcErr.StatusCode).To(Equal(0))
+			// the error returned by the HTTP layer seems to be inconsistent between
+			// "Client.Timeout exceeded" and "context deadline exceeded" for some reason,
+			// I did not dig into it further but it makes it hard to properly test the
+			// properties about the returned error, so just checking that there is an
+			// error and that we didn't wait much more than the timeout.
+			Expect(ctx.Err()).To(BeNil())
+			Expect(duration).To(BeNumerically(">=", 900*time.Millisecond))
+			Expect(duration).To(BeNumerically("<=", 1100*time.Millisecond))
+		})
+	})
 })